feat: add validation to `PartialMmr` deserialization and `from_parts` by Farukest · Pull Request #812 · 0xMiden/crypto

Farukest · 2026-01-31T16:22:27Z

Summary

Adds validation to PartialMmr::from_parts() to ensure consistency between components
Adds from_parts_unchecked() for performance-critical trusted code paths
Updates Deserializable implementation to use the validating constructor

Validation Checks

track_latest can only be true when forest has a single leaf tree
All node indices must be within forest bounds

Note

PartialMmr::from_parts and Deserializable did not validate consistency between components. This is a security concern when deserializing from untrusted sources (e.g., via ProvenTransaction).

Changes

error.rs: Added InconsistentPartialMmr error variant
partial.rs: from_parts() now returns Result<Self, MmrError> with validation
partial.rs: Added from_parts_unchecked() for trusted/performance-critical paths
partial.rs: Deserializable now validates via MmrPeaks::new() and from_parts()

Closes #802

miden-crypto/src/merkle/mmr/partial.rs

huitseeker · 2026-02-03T12:20:12Z

miden-crypto/src/merkle/mmr/partial.rs

+    ) -> Result<Self, MmrError> {
+        let forest = peaks.forest();
+
+        // Validate track_latest: can only be true if forest has an odd element (single leaf tree)


The new check rejects inputs where track_latest=true but the forest has no single-leaf tree.

This breaks old data that set the flag loosely.

This validation is intentional for security (untrusted deserialization from #802).
If track_latest=true but there's no single-leaf tree, the data is logically inconsistent, there's no "latest leaf" to track.
For trusted sources or migration paths, from_parts_unchecked() is available which skips all validation.
Should we add a note in the docs about this breaking change for invalid data ?

We should perhaps just mark the change as breaking in the Changelog.

huitseeker · 2026-02-03T12:20:16Z

miden-crypto/src/merkle/mmr/partial.rs

+        let result = PartialMmr::from_parts(peaks_even.clone(), BTreeMap::new(), false);
+        assert!(result.is_ok());
+
+        // Invalid case: node index out of bounds


The test only checks leaf nodes. Please add tests for:

Index 0 (invalid)

Large even indices beyond the forest range

Bad inputs during deserialization

These matter because internal nodes are used for auth paths.

Farukest · 2026-02-03T16:34:19Z

All done @huitseeker. kindly need review again.

huitseeker

This is shaping up, thanks!

huitseeker · 2026-02-04T23:37:53Z

miden-crypto/src/merkle/mmr/partial.rs

+        // For an empty forest, no nodes are valid
+        if !nodes.is_empty() && forest.is_empty() {
+            return Err(MmrError::InconsistentPartialMmr(
+                "nodes present but forest is empty".into(),


The current validation only checks that indices are within rightmost_in_order_index(), but this includes "separator" positions between trees in multi-tree forests. For example, in a forest with 7 leaves (0b111), index 6 falls between trees but would pass validation even though it does not correspond to a real MMR node.

These separator indices are not valid for NodeMap. See:

track() only inserts at idx.sibling() where idx starts from a valid leaf position

add() only inserts at positions derived from rightmost_in_order_index() and its sibling

The NodeMap documentation states it contains "authentication nodes" which are always real tree nodes, never separators

Consider adding a Forest::is_valid_in_order_index() helper that checks if an index falls within an actual tree span rather than a separator gap.

huitseeker · 2026-02-04T23:37:57Z

miden-crypto/src/merkle/mmr/partial.rs

+        let result = PartialMmr::from_parts(peaks.clone(), BTreeMap::new(), true);
+        assert!(result.is_ok());
+
+        // Build an MMR with 8 leaves (no single leaf tree: 0b1000)


The tests don't cover the case where an index is in-range but points to a separator between trees. See the comment above on that topic.

huitseeker · 2026-02-04T23:38:01Z

miden-crypto/src/merkle/mmr/partial.rs

+        assert!(result.is_err());
+
+        // Invalid case: index 0 (which is never valid for InOrderIndex)
+        let mut nodes_with_zero = BTreeMap::new();


The comment says the rightmost in-order index for 7 leaves is 12, but Forest::new(0b0111).rightmost_in_order_index() returns 13. The test at line 191 in tests.rs confirms this. Please fix or remove the comment to avoid confusion.

huitseeker · 2026-02-04T23:49:04Z

miden-crypto/src/merkle/mmr/partial.rs

+    ) -> Result<Self, MmrError> {
+        let forest = peaks.forest();
+
+        // Validate track_latest: can only be true if forest has an odd element (single leaf tree)


We should perhaps just mark the change as breaking in the Changelog.

Farukest · 2026-02-05T05:15:31Z

All done @huitseeker 👍

huitseeker

This LGTM!

This commit adds validation to `PartialMmr::from_parts()` and the `Deserializable` implementation to ensure consistency between components: - Validates that `track_latest` is only true when forest has a single leaf tree - Validates that all node indices are within forest bounds - Adds `from_parts_unchecked()` for performance-critical trusted code paths - Updates `Deserializable` to use the validating constructor This addresses security concerns when deserializing from untrusted sources. Closes 0xMiden#802

Address review feedback: - Reject index 0 as invalid (InOrderIndex starts at 1) - Check all indices against forest.rightmost_in_order_index() - Handle empty forest case explicitly - Add tests for index 0, large even indices, and deserialization

- Add Forest::is_valid_in_order_index() to check if an index points to an actual node (not a separator position between trees) - Update from_parts() to reject separator indices - Add tests for separator index validation (indices 8 and 12 for 7-leaf forest) - Fix comment: rightmost in-order index for 7 leaves is 13, not 12 - Mark PR as [BREAKING] in CHANGELOG

krushimir · 2026-02-14T16:24:26Z

miden-crypto/src/merkle/mmr/tests.rs

+    // Single tree forests (power of 2 leaves) have no separators
+    // Forest with 1 leaf: valid indices are just 1
+    let forest_1 = Forest::new(0b0001);
+    assert!(!forest_1.is_valid_in_order_index(&idx(2)), "index 0 is invalid");


Suggested change

assert!(!forest_1.is_valid_in_order_index(&idx(2)), "index 0 is invalid");

assert!(!forest_1.is_valid_in_order_index(&idx(2)), "index 2 is invalid");

krushimir

Left a few minor nits but nothing blocking. LGTM!

krushimir · 2026-02-14T16:31:33Z

miden-crypto/src/merkle/mmr/partial.rs

+
+    /// Returns a new [PartialMmr] instantiated from the specified components without validation.
+    ///
+    /// # Safety


nit: # Safety is conventionally used for unsafe fn in Rust docs. Since this is a safe function, maybe # Correctness or # Preconditions fits better?

krushimir · 2026-02-14T16:41:43Z

miden-crypto/src/merkle/mmr/partial.rs

+    /// - All tracked leaf positions must be within forest bounds.
+    /// - All tracked leaves must have their values in the nodes map.
+    /// - All node indices must be valid leaf or internal node positions within the forest.
+    ///


nit: it might be worth noting in the doc that this is structural validation only. Just so callers don't assume Ok means every tracked leaf is openable via open().

Something like:

/// Note: This performs structural validation only. It does not verify that authentication /// paths for tracked leaves are complete or that node values are cryptographically /// consistent with the peaks.

Farukest mentioned this pull request Jan 31, 2026

Add validation to PartialMmr deserialization and from_parts #802

Open

Farukest force-pushed the feat/partial-mmr-validation branch from 61461bb to 0724921 Compare January 31, 2026 16:25

huitseeker reviewed Feb 3, 2026

View reviewed changes

Farukest force-pushed the feat/partial-mmr-validation branch from 2f03f9f to d9d8176 Compare February 3, 2026 14:27

huitseeker reviewed Feb 4, 2026

View reviewed changes

huitseeker requested a review from iamrecursion February 9, 2026 01:01

huitseeker approved these changes Feb 9, 2026

View reviewed changes

iamrecursion removed their request for review February 10, 2026 08:44

huitseeker requested a review from krushimir February 11, 2026 13:51

Farukest added 2 commits February 14, 2026 09:05

Farukest force-pushed the feat/partial-mmr-validation branch from 9f3ce04 to fbd1ce9 Compare February 14, 2026 06:09

Farukest force-pushed the feat/partial-mmr-validation branch from fbd1ce9 to f78b142 Compare February 14, 2026 10:10

huitseeker requested review from bobbinth and gswirski February 14, 2026 14:11

krushimir reviewed Feb 14, 2026

View reviewed changes

krushimir approved these changes Feb 14, 2026

View reviewed changes

Merge branch 'next' into feat/partial-mmr-validation

45c174a

	assert!(!forest_1.is_valid_in_order_index(&idx(2)), "index 0 is invalid");
	assert!(!forest_1.is_valid_in_order_index(&idx(2)), "index 2 is invalid");

Conversation

Farukest commented Jan 31, 2026

Summary

Validation Checks

Note

Changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Farukest commented Feb 3, 2026

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

huitseeker Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Farukest commented Feb 5, 2026

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krushimir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

huitseeker Feb 4, 2026 •

edited

Loading